Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 7dab62c in 27 seconds. Click for details.
- Reviewed
278lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_qdhrWTbgua3m9dL0
You can customize 
Greptile OverviewGreptile SummaryThis PR implements node retention functionality to track and propagate signal source origins through the ABC optimization flow. Changes:
Issues:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Yosys
participant BlifParser as blifparse.cc
participant AbcPass as abc.cc
participant ABC as ABC Tool
User->>Yosys: Set abc.node_retention flag
User->>Yosys: Run ABC pass
Note over Yosys,ABC: Export Phase
Yosys->>AbcPass: prepare_module()
AbcPass->>AbcPass: Check abc_node_retention flag
alt node_retention enabled
AbcPass->>AbcPass: Add "-r" to read_blif command
end
AbcPass->>ABC: Export to input.blif
AbcPass->>ABC: run read_blif [-r] "input.blif"
ABC->>ABC: Process with node retention
ABC->>AbcPass: Write output.blif with node retention data
Note over Yosys,ABC: Import Phase
AbcPass->>BlifParser: parse_blif(output.blif)
BlifParser->>BlifParser: Parse .node_retention_begin block
loop for each node_name SRC sources
BlifParser->>BlifParser: Find wire by node_name
BlifParser->>BlifParser: Set \node_retention_sources attribute
end
BlifParser->>BlifParser: Parse .node_retention_end
BlifParser->>AbcPass: Return parsed design
Note over Yosys,ABC: Re-integration Phase
AbcPass->>AbcPass: extract() - re-integrate ABC results
loop for each wire with \node_retention_sources
AbcPass->>AbcPass: Parse sources string
loop for each source node
AbcPass->>AbcPass: remap_name(source, &orig_wire)
AbcPass->>AbcPass: Add orig_wire src to pool
end
AbcPass->>AbcPass: wire->add_strpool_attribute(src, pool)
end
loop for each cell
AbcPass->>AbcPass: Get output wire's src pool
AbcPass->>AbcPass: cell->add_strpool_attribute(src, pool)
AbcPass->>Yosys: design->select(module, cell)
end
AbcPass->>Yosys: Return optimized module
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
passes/techmap/abc.cc
Outdated
| Wire *out_wire = c->getPort((c->hasPort(ID::Y)) ? ID::Y : ID::Q).as_wire(); | ||
| Wire *remapped_out_wire = module->wire(remap_name(out_wire->name)); | ||
| std::string src_attribute = sig2src[remapped_out_wire]; | ||
| log("Processing cell %s\n", c->name.c_str()); |
There was a problem hiding this comment.
this debug log will print for every cell in mapped design - can create excessive output. Consider using log_debug() or removing
Prompt To Fix With AI
This is a comment left during a code review.
Path: passes/techmap/abc.cc
Line: 1493:1493
Comment:
this debug log will print for every cell in mapped design - can create excessive output. Consider using `log_debug()` or removing
How can I resolve this? If you propose a fix, please make it concise.
passes/techmap/abc.cc
Outdated
| } | ||
| wire->add_strpool_attribute(ID::src, src_pool); | ||
| } else { | ||
| log("No node retention sources found for wire %s\n", w->name.c_str()); |
There was a problem hiding this comment.
this log will print for every wire without node retention sources - likely excessive output. Consider using log_debug() or only logging when expected
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: passes/techmap/abc.cc
Line: 1480:1480
Comment:
this log will print for every wire without node retention sources - likely excessive output. Consider using `log_debug()` or only logging when expected
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Should this ever happen? If not, this should be a warning not INFO. If it happens a lot, it should be debug
There was a problem hiding this comment.
Based on the context, this should be log_debug().
Node retention sources are only present on specific wires that have node retention information from .node_retention_begin/.node_retention_end blocks in BLIF files. In a typical design, most wires won't have this specialized attribute, so the current INFO log would fire for the majority of wires, creating excessive noise.
| log("No node retention sources found for wire %s\n", w->name.c_str()); | |
| } else { | |
| log_debug("No node retention sources found for wire %s\n", w->name.c_str()); |
This keeps the information available for debugging while avoiding cluttering the output during normal operation.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: passes/techmap/abc.cc
Line: 1150:1157
Comment:
removed error handling for ABC script failures - if ABC prints an error and returns to prompt, this will now hang waiting for `YOSYS_ABC_DONE` instead of failing gracefully. Was this removal intentional?
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 462caed in 40 seconds. Click for details.
- Reviewed
3963lines of code in17files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_U2CqIYM513oNWT60
You can customize
passes/techmap/abc.cc
Outdated
| } | ||
| wire->add_strpool_attribute(ID::src, src_pool); | ||
| } else { | ||
| log("No node retention sources found for wire %s\n", w->name.c_str()); |
There was a problem hiding this comment.
Should this ever happen? If not, this should be a warning not INFO. If it happens a lot, it should be debug
|
I think your blifparse removal is causing the tests to fail... please double check |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 625d2ca in 13 seconds. Click for details.
- Reviewed
7lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_x2ZPQi4Eag7JWFWw
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 47469c2 in 19 seconds. Click for details.
- Reviewed
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_3oP6XmGrj1nBWCwK
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed f452702 in 20 seconds. Click for details.
- Reviewed
30lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_AWZHJZyQzaHs9JMf
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 498f74e in 16 seconds. Click for details.
- Reviewed
7lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_n9IakKrZirdgHhKb
You can customize
AdvaySingh1
force-pushed
the
nr_cleanup
branch
from
498f74e to
1d99104
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 1d99104 in 23 seconds. Click for details.
- Reviewed
20lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_4CAET1HEzNYL0VjF
You can customize
AdvaySingh1
force-pushed
the
nr_cleanup
branch
from
1d99104 to
07e21e7
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 07e21e7 in 13 seconds. Click for details.
- Reviewed
20lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_XR8rfab6Ofcpheic
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 1317cbb in 16 seconds. Click for details.
- Reviewed
7lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_dkgZFJPs9sQE5v8F
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2f3ed06 in 14 seconds. Click for details.
- Reviewed
7lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_1V6DNT9xUBGC1H3B
You can customize
|
@greptile please review |
frontends/blif/blifparse.cc
Outdated
| blif_maxnum = 0; | ||
| } | ||
|
|
||
| // Parse optional node retention section that tracks signal source origins for ABC reintegration. |
There was a problem hiding this comment.
Can you indent this to be consistent with everything surrounding?
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…case cells Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed d097e53 in 37 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_8h1VDeyZwchWQACq
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 0b96050 in 28 seconds. Click for details.
- Reviewed
23lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_1S9O8FfRUrJv6SX1
You can customize
|
Will update the abc submodule when that PR is 100% ready |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 4302772 in 19 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_8mJ9RmWwy5guCdU6
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 607ef02 in 15 seconds. Click for details.
- Reviewed
29lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_B92G8LhAM5WC3yQT
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 8d22f6d in 12 seconds. Click for details.
- Reviewed
1501lines of code in28files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_QYjocc3Mt07vLywD
You can customize
2 participants
Node retention PR.
Changed blifparse, abc.cc, and abc submodule.
Important
Adds node retention parsing and propagation in Yosys synthesis tool to track and annotate signal source origins.
.node_retention_beginand.node_retention_endsections inparse_blif()inblifparse.ccto track signal source origins.prepare_module()inabc.ccto include node retention options in the ABC script.extract()inabc.ccto propagate node retention sources to wire attributes.abc_node_retentionandabc_max_node_retention_originsoptions inAbcConfiginabc.ccto control node retention behavior.82fdc5a81df8dcdd798eea56ecdc9f5a24f32813.This description was created by
for 8d22f6d. You can customize this summary. It will automatically update as commits are pushed.